Skip to content

Align jedi_lfric_tests linear model/adjoint testing to adjoint_tests and linear_model#156

Open
tom-j-h wants to merge 51 commits intoMetOffice:mainfrom
tom-j-h:align_jedi_lfric_tests_to_linear_model
Open

Align jedi_lfric_tests linear model/adjoint testing to adjoint_tests and linear_model#156
tom-j-h wants to merge 51 commits intoMetOffice:mainfrom
tom-j-h:align_jedi_lfric_tests_to_linear_model

Conversation

@tom-j-h
Copy link
Contributor

@tom-j-h tom-j-h commented Jan 19, 2026

PR Summary

Sci/Tech Reviewer: @ss421
Code Reviewer: @mike-hobson

PLEASE NOTE - this is a follow-on to #132. The branch was created from #132's branch in my fork, but I can't make a PR into that branch because then I would be stuck in my fork. So, to look at the actual changes relevant to this PR alone, look at the diff of this branch with #132's branch: tom-j-h/lfric_apps@jelf_adjoint_test_tolerance_nml...tom-j-h:lfric_apps:align_jedi_lfric_tests_to_linear_model

Similar to #84, but for applications of jedi_lfric_tests that use the linear and adjoint models.

This change relies on the same small code change in science/adjoint as #84. It is also very useful to be able to set adjoint test tolerance via a namelist variable () for running the strict vs. relaxed adjoint tests, hence the order of branching.

A bug (incorrect adjoint) relating to the incremental wind interpolation added in https://code.metoffice.gov.uk/trac/lfric_apps/ticket/369 has arisen due to the changes here. The specific cause has not been identified. For now, this is switched off. This is not high priority, hence not investigating and fixing it as part of this work. Have opened #128.

The same approach to handling the bug highlighted in #84 (rrt_equals_dt) is taken here; this has issue #87.

Code Quality Checklist

  • I have performed a self-review of my own code
  • My code follows the project's style guidelines
  • Comments have been included that aid understanding and enhance the readability of the code
  • My changes generate no new warnings
  • All automated checks in the CI pipeline have completed successfully

Testing

  • I have tested this change locally, using the LFRic Core rose-stem suite
  • If required (e.g. API changes) I have also run the LFRic Apps test suite using this branch
  • If any tests fail (rose-stem or CI) the reason is understood and acceptable (e.g. kgo changes)
  • I have added tests to cover new functionality as appropriate (e.g. system tests, unit tests, etc.)
  • Any new tests have been assigned an appropriate amount of compute resource and have been allocated to an appropriate testing group (i.e. the developer tests are for jobs which use a small amount of compute resource and complete in a matter of minutes)

trac.log

I can't find my trac.log! It might be because I ran with --no-run-name? Here's the Cylc Review: https://cylchub/services/cylc-review/taskjobs/tom.hill/?suite=align_jedi_lfric_tests_to_linear_model-developer.

Security Considerations

  • I have reviewed my changes for potential security issues
  • Sensitive data is properly handled (if applicable)
  • Authentication and authorisation are properly implemented (if applicable)

Performance Impact

  • Performance of the code has been considered and, if applicable, suitable performance measurements have been conducted

AI Assistance and Attribution

  • Some of the content of this change has been produced with the assistance of Generative AI tool name (e.g., Met Office Github Copilot Enterprise, Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the Simulation Systems AI policy (including attribution labels)

Documentation

  • Where appropriate I have updated documentation related to this change and confirmed that it builds correctly

PSyclone Approval

  • If you have edited any PSyclone-related code (e.g. PSyKAl-lite, Kernel interface, optimisation scripts, LFRic data structure code) then please contact the TCD Team

Sci/Tech Review

  • I understand this area of code and the changes being added
  • The proposed changes correspond to the pull request description
  • Documentation is sufficient (do documentation papers need updating)
  • Sufficient testing has been completed

(Please alert the code reviewer via a tag when you have approved the SR)

Code Review

  • All dependencies have been resolved
  • Related Issues have been properly linked and addressed
  • CLA compliance has been confirmed
  • Code quality standards have been met
  • Tests are adequate and have passed
  • Documentation is complete and accurate
  • Security considerations have been addressed
  • Performance impact is acceptable

tom-j-h and others added 22 commits January 7, 2026 10:23
…j-h/lfric_apps into align_adjoint_tests_to_linear_model
@tom-j-h tom-j-h changed the title Align jedi lfric tests to linear model Align jedi_lfric_tests linear model/adjoint testing to adjoint_tests and linear_model Jan 19, 2026
@tom-j-h
Copy link
Contributor Author

tom-j-h commented Jan 27, 2026

Have added responses to inline comments now, and pushed related changes.

@ss421
Copy link

ss421 commented Jan 28, 2026

Some of your comments (those to do with adjoint_tests) are to do with changes that are not part of this PR - rather than looking at the "Files changed" section above, I suggest to look at the diff of this PR's branch with the previous PR's:

PLEASE NOTE - this is a follow-on to #132. The branch was created from #132's branch in my fork, but I can't make a PR into that branch because then I would be stuck in my fork. So, to look at the actual changes relevant to this PR alone, look at the diff of this branch with #132's branch: tom-j-h/lfric_apps@jelf_adjoint_test_tolerance_nml...tom-j-h:lfric_apps:align_jedi_lfric_tests_to_linear_model

Sorry - I did use your diff but managed to identify the wrong file when adding my comment, sorry for the confusion.

The reason there are some configuration options changed even though they don't actually have an effect is that previously there was a request to have the jedi_lfric_tests configurations match the linear_model/adjoint_tests ones exactly (except for JEDI-specific bits). If you'd rather those parts were unchanged then I'll do that instead. I see in the case of ls_option that it should definitely be different if it results in unnecessary code being run - thanks for pointing this out

The issue is the jedi specific bits are not obvious because we are not able to add that into the configuration. What we are doing is safe but it is a little confusing.

Similarly, the reason nwp_gal9 is done as an optional configuration which is included by default, is that this is how linear_model and adjoint_tests does it.

Thanks, yes Ive looked at the update. The nwp optional part is just pointing at newer files so I think the base configuration is effectively the nwp-gal9. Happy with what is done here now I've looked at the linear_model. I could have done that in the first place :-(.

As for the canned namelists, these were updated simply by copying over the generated namelists from rose-stem tests. I didn't spot those unnecessary differences you commented on - thanks.

Thanks, yes I assumed that was the case.

As for the other applications in jedi_lfric_tests other than jedi_tlm_tests and jedi_id_tlm_tests, I'm not really sure what these are for so left them alone. What do you suggest I do? I'm wary of it ending up being a lot of work with the deadline coming up.

Regarding the configurations:

rose-stem/app/jedi_lfric_tests test effectively runs the linear_model so should be based on the linear configurations. This should be a direct copy (for nwp_gal9 and runge-kutta) - the app is setup in the same way as the linear model (i.e. does not run via the emulator)

rose-stem/app/jedi_tlm_forecast_tl runs the linear_model via the emulator so should have similar changes to jedi_tlm_tests and jedi_id_tlm_tests

The other apps (jedi_forecast_pseudo and jedi_forecast) are based on the linear but they do not need to be kept in line with the linear.

It would be good to update jedi_tlm_forecast_tl and jedi_lfric_tests but its not essential since time is short. It can be done as a follow on.

I looked at the diff again tom-j-h/lfric_apps@jelf_adjoint_test_tolerance_nml...tom-j-h:lfric_apps:align_jedi_lfric_tests_to_linear_model, I noticed a few things that I mentioned in previous note that have not been updated. I think mainly due to confusion with me picking the wrong file. I add the notes here, can take a look?

  1. I think the rose-app-default.conf can be removed?

  2. In the configurations *.nml and *.conf, can you check:

i. File section looks like:

&files
ancil_directory='/data/users/lfricadmin/data/ancils/basic-gal/yak/C12',
checkpoint_stem_name='',
diag_stem_name='',
orography_mean_ancil_path='orography/gmted_ramp2/qrparm.orog',
start_dump_directory='',
start_dump_filename='',

ii. Initialization has:

init_option='analytic',
ls_option='analytic',

iii. Partitioning has (not sure how much it matters...):

&partitioning
panel_xproc=1,
panel_yproc=1,

@tom-j-h tom-j-h added the KGO This PR contains changes to KGO label Jan 28, 2026
@tom-j-h
Copy link
Contributor Author

tom-j-h commented Jan 28, 2026

Thanks Steve, everything addressed now, including updating configs for jedi_lfric_tests and jedi_tlm_forecast_tl. jedi_lfric_tests_developer group all okay except KGOs need updating: https://cylchub/services/cylc-review/taskjobs/tom.hill/?suite=align_jedi_lfric_tests_to_linear_model-jedi_lfric_tests_developer%2Frun2

Ready for rereview.

@ss421
Copy link

ss421 commented Jan 28, 2026

Thanks Steve, everything addressed now, including updating configs for jedi_lfric_tests and jedi_tlm_forecast_tl. jedi_lfric_tests_developer group all okay except KGOs need updating: https://cylchub/services/cylc-review/taskjobs/tom.hill/?suite=align_jedi_lfric_tests_to_linear_model-jedi_lfric_tests_developer%2Frun2

Ready for rereview.

Thanks for the updates. Happy with the changes but note that the KGO needs updating so some KGO checks are failing. This is due to an update to the configuration so it is expected and Im happy for the new kgo's to be added. Im not sure of the process related to that. Should that be done before sending to CR? @mike-hobson @tom-j-h

@github-actions github-actions bot requested a review from mike-hobson January 28, 2026 12:51
@tom-j-h
Copy link
Contributor Author

tom-j-h commented Jan 28, 2026

Thanks for bearing with me @ss421 ! I believe KGOs are updated by the Code Reviewer.

@ss421
Copy link

ss421 commented Jan 28, 2026

Looks like the ssd-bot has spotted that this can move to CR :-) now with @mike-hobson

Copy link

@ss421 ss421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing another change (#182 (comment)) I noticed that I missed a few places where the configuration was modified. These are example canned test configurations so they do not impact on testing and should not impact on CR. Apologies for missing these.

@ss421
Copy link

ss421 commented Jan 30, 2026

This requires a small JEDI update. @tom-j-h will provide this. Captured in the next release PR here: https://github.com/JCSDA-internal/lfric-jedi/pull/1206

@james-bruten-mo
Copy link
Collaborator

#132 is on which I think unblocks this one to be brought up to date and reviewed

@tom-j-h
Copy link
Contributor Author

tom-j-h commented Feb 6, 2026

#132 is on which I think unblocks this one to be brought up to date and reviewed

Thanks, now updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The CLA has been signed as part of this PR - added by GA KGO This PR contains changes to KGO

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Align jedi_lfric_tests linear model/adjoint testing to adjoint_tests/linear_model

4 participants